Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.86%. Comparing base (a48c297) to head (93d7c24).
⚠️ Report is 4 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3130   +/-   ##
=======================================
  Coverage   96.86%   96.86%           
=======================================
  Files          22       22           
  Lines       10081    10081           
  Branches     1824     1824           
=======================================
  Hits         9765     9765           
  Misses        267      267           
  Partials       49       49           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The code that changes behavior with this PR is something like:

obj = {
    get largestUnit() {
        console.log("get largestUnit");
        return "years";
    },
    get smallestUnit() {
        console.log("get smallestUnit");
        return "seconds";
    },
}

Temporal.PlainTime.from("00:00").round(obj)

Current behavior: console logs "get largestUnit" and then RangeError is thrown.

New behavior: console logs both "get largestUnit" and "get smallestUnit", and then RangeError is thrown.

Note: the RangeError is because PlainTime does not allow "years" as a largestUnit. There is no difference in behavior if get largestUnit() returns an invalid string such as "INVALID".

(as noted previously, there doesn't appear to be Test262 coverage for this)

This PR fixes the issues involving unit-valued options, which is the current scope of the issue and all I'm asking for at this time.

Yes, often smallestUnit is the final read, so no change was needed in those cases.

Yes, splitting the editorial change into its own PR is something I'm happy to do.

@anba Do you have any concerns about this change?

I opened #3135 for the editorial changes.

The editorial changes are merged, and I rebased this. So now it's just the minimal set of normative changes.

@anba Do you have any concerns about this change?

I don't think so. It'll require some small refactoring because the current SM implementation sometimes uses AUTO as a placeholder to represent absent values. This approach is no longer possible when "auto" must always initially be parsed as a valid Temporal unit option. But apart from that I don't see any implementation issues.

(The SM implementation already parses the unit options accepting all units and only later check which units are actually allowed. )

I wonder is there a difference between "Get-Cast-Get-Cast-Validate" and "Get-Get-Cast-Cast-Validate"

I wonder is there a difference between "Get-Cast-Get-Cast-Validate" and "Get-Get-Cast-Cast-Validate"

Yes, there is, the Get step may fail if the object has an accessor property, and the Cast step may fail if the property is of the wrong type, so it's observable.

I'd like to write test262 tests for this before merging it, because it's not covered at all. To that end, it'd be helpful if #3142 could be reviewed and merged first, as it contains the updates to the reference code for the editorial changes.

I wonder is there a difference between "Get-Cast-Get-Cast-Validate" and "Get-Get-Cast-Cast-Validate"

Yes, there is, the Get step may fail if the object has an accessor property, and the Cast step may fail if the property is of the wrong type, so it's observable.

I'd like to know why you chose "Get-Cast-Get-Cast-Validate", not "Get-Get-Cast-Cast-Validate". Is there a difference between them when making a fast implementation? If there is no difference, "Get-Get-Cast-Cast-Validate" looks clearer to me.

I wonder is there a difference between "Get-Cast-Get-Cast-Validate" and "Get-Get-Cast-Cast-Validate"

Yes, there is, the Get step may fail if the object has an accessor property, and the Cast step may fail if the property is of the wrong type, so it's observable.

I'd like to know why you chose "Get-Cast-Get-Cast-Validate", not "Get-Get-Cast-Cast-Validate". Is there a difference between them when making a fast implementation? If there is no difference, "Get-Get-Cast-Cast-Validate" looks clearer to me.

The thinking was that Get and Cast are both steps that deeply involve the JS bindings, a layer above the library boundary, whereas Validate is on the library layer. Also, the champions didn't want to make a large change, and Get Get Cast Cast would involve rewriting all call sites of GetOption.

I've written tests to cover this in tc39/test262#4557. Please check if the behaviour is as you expect.

Is the handling of Auto correct here?

@sffc I am trying to implement this, and I noted that GetTemporalUnitValuedOption unconditionally parses "auto", however no call for Validate has auto as extraValues, AND auto is not in the table, so AIUI Validate will always reject Auto. Probably a bug?

We think that the only case where ~auto~ is allowed is a case where everything is allowed, and so we removed the validate call entirely, in #3143

I don't think there's a bug?
largestUnit: 'auto' is accepted in since, until, and Duration.round methods. In GetDifferenceSettings auto is given in extraValues here. In Duration.round, no call to Validate is needed because all the possible values are valid, see #3130 (comment).

Ah! I was looking at calls to Validate, so I didn't notice there was a case without a call. Thanks!

Might make a PR to add this to the description.

There is one other call site of validate that permits auto, in GetDifferenceSettings

I've written tests to cover this in tc39/test262#4557. Please check if the behaviour is as you expect.

I can verify that V8 updated to this PR passes those tests

It'll be easiest if I can get a review of #3145 before merging this one, because it's convenient to pull in updated test262 commits in order.